Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed binding of command line flags #1129

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

jpkrohling
Copy link
Contributor

Fixes #1128 by binding the flags to Viper as part of the command execution, not during the building of the Command object.

Signed-off-by: Juraci Paixão Kröhling [email protected]

@jpkrohling
Copy link
Contributor Author

This can be reproduced by running make run-debug. Before this PR, no DEBUG statements would show up, indicating that the log-level was not changed:

$ make run-debug
INFO[0000] Versions                                      arch=amd64 identity=default.jaeger-operator jaeger=1.18.1 jaeger-operator=v1.18.1-10-g11a063f9 operator-sdk=v0.15.1 os=linux version=go1.14.3
INFO[0000] Auto-detected the platform                    platform=kubernetes
INFO[0000] Auto-detected ingress api                     ingress-api=networking
INFO[0000] Automatically adjusted the 'es-provision' flag  es-provision=no
INFO[0000] Automatically adjusted the 'kafka-provision' flag  kafka-provision=no
INFO[0000] The service account running this operator has the role 'system:auth-delegator', enabling OAuth Proxy's 'delegate-urls' option 
INFO[0001] Install prometheus-operator in your cluster to create ServiceMonitor objects  error="no ServiceMonitor registered with the API"
WARN[0002] failed to setup the Jaeger exporter           error="write udp 127.0.0.1:47051->127.0.0.1:6831: write: connection refused"
^CFATA[0004] <nil>                                        
exit status 1

After this PR, there's one DEBUG statement:

$ make run-debug
INFO[0000] Versions                                      arch=amd64 identity=default.jaeger-operator jaeger=1.18.1 jaeger-operator=v1.18.1-11-g9e5a0cd2 operator-sdk=v0.15.1 os=linux version=go1.14.3
INFO[0000] Auto-detected the platform                    platform=kubernetes
INFO[0000] Auto-detected ingress api                     ingress-api=networking
INFO[0000] Automatically adjusted the 'es-provision' flag  es-provision=no
INFO[0000] Automatically adjusted the 'kafka-provision' flag  kafka-provision=no
INFO[0000] The service account running this operator has the role 'system:auth-delegator', enabling OAuth Proxy's 'delegate-urls' option 
INFO[0001] Install prometheus-operator in your cluster to create ServiceMonitor objects  error="no ServiceMonitor registered with the API"
DEBU[0001] Not running on OpenShift, so won't configure OAuthProxy imagestream. 
WARN[0002] failed to setup the Jaeger exporter           error="write udp 127.0.0.1:35449->127.0.0.1:6831: write: connection refused"
^CFATA[0002] <nil>                                        
exit status 1

This can also be easily reproduced/verified by setting platform to openshift when using minikube.

@jpkrohling
Copy link
Contributor Author

@chlunde, would you like to review this one as well?

pkg/cmd/start/main.go Outdated Show resolved Hide resolved
pkg/service/collector.go Outdated Show resolved Hide resolved
Fixes jaegertracing#1128

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling force-pushed the jpkrohling/issue1128 branch from 852a39c to aa5c4b1 Compare July 10, 2020 14:09
@jpkrohling jpkrohling requested a review from pavolloffay July 10, 2020 14:12
@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #1129 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1129   +/-   ##
=======================================
  Coverage   88.22%   88.22%           
=======================================
  Files          86       86           
  Lines        5350     5351    +1     
=======================================
+ Hits         4720     4721    +1     
  Misses        466      466           
  Partials      164      164           
Impacted Files Coverage Δ
pkg/apis/jaegertracing/v1/jaeger_types.go 100.00% <ø> (ø)
pkg/service/collector.go 100.00% <ø> (ø)
pkg/autodetect/main.go 82.45% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd60752...aa5c4b1. Read the comment docs.

Copy link
Contributor

@kevinearls kevinearls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This fixes the problem I had with it ignoring kafka-provisioning-minimal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command line flags are not being handled properly
3 participants